Fix external payload validation logic, resolve txs execution logic bug and add pre-exec validation #341
Conversation
sieniven
requested review from
0x416e746f6e,
SozinM,
avalonche and
karim-agha
as code owners
|
hi @sieniven thank you for the fixes, apologies for the slow response! are you able to share the steps you used to trigger the hash mismatch error? this feature is very experimental and has only been tested locally, not on any sort of production environment. it would be good to have the cases you used for testing documented. |
Hey @noot, thanks for taking the time to explain - this makes sense. The hash mismatch error occur in 2 scenarios. I'll comment directly on the code blocks for your reference. |
| let sender = tx | ||
| .recover_signer() | ||
| .wrap_err("failed to recover tx signer")?; | ||
| let tx_env = TxEnv::from_recovered_tx(&tx, sender); | ||
| let executable_tx = match tx { | ||
| OpTxEnvelope::Deposit(ref tx) => { | ||
| let deposit = DepositTransactionParts { | ||
| mint: Some(tx.mint), | ||
| source_hash: tx.source_hash, | ||
| is_system_transaction: tx.is_system_transaction, | ||
| }; | ||
| OpTransaction { | ||
| base: tx_env, | ||
| enveloped_tx: None, | ||
| deposit, | ||
| } | ||
| } | ||
| OpTxEnvelope::Legacy(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| OpTxEnvelope::Eip2930(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| OpTxEnvelope::Eip1559(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| OpTxEnvelope::Eip7702(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| }; |
There was a problem hiding this comment.
The first problematic area that the PR resolves is here - there shouldnt be a need to write local definition of convertion methods to alloy typed tx types, but instead pass the tx value itself directly into evm.transact, which allows the safe conversion into the tx env interface (for revm execution) using upstream conversion methods.
| .transpose() | ||
| .wrap_err("failed to get depositor nonce")?; | ||
|
|
||
| let ResultAndState { result, state } = match evm.transact_raw(executable_tx) { |
There was a problem hiding this comment.
Better to use the upstream's default pattern, evm.transact here which inherently calls IntoTxEnv on the concrete type for tx execution.
| let depositor_nonce = (is_regolith_active && tx.is_deposit()) | ||
| .then(|| { | ||
| evm.db_mut() | ||
| .load_cache_account(sender) | ||
| .map(|acc| acc.account_info().unwrap_or_default().nonce) | ||
| }) | ||
| .transpose() | ||
| .wrap_err("failed to get depositor nonce")?; |
There was a problem hiding this comment.
The second issue (and where the block hash mismatch is) is located here. The depositor nonce when bulding the receipt should be using the nonce prior to execution. However, the deposit tx receipt uses the depositor nonce after execution, which will cause a stateroot mismatch on the incorrect block data
| /// Validates the payload header and its relationship with the parent before execution. | ||
| /// This performs consensus rule validation including: | ||
| /// - Header field validation (timestamp, gas limit, etc.) | ||
| /// - Parent relationship validation (block number increment, timestamp progression) | ||
| fn validate_pre_execution( | ||
| payload: &OpBuiltPayload, | ||
| parent_header: &reth_primitives_traits::Header, | ||
| parent_hash: alloy_primitives::B256, | ||
| chain_spec: Arc<OpChainSpec>, | ||
| ) -> eyre::Result<()> { | ||
| use reth::consensus::HeaderValidator; | ||
|
|
||
| fn is_regolith_active(chain_spec: &OpChainSpec, timestamp: u64) -> bool { | ||
| use reth_optimism_chainspec::OpHardforks as _; | ||
| chain_spec.is_regolith_active_at_timestamp(timestamp) | ||
| let consensus = OpBeaconConsensus::new(chain_spec); | ||
| let parent_sealed = SealedHeader::new(parent_header.clone(), parent_hash); | ||
|
|
||
| // Validate incoming header | ||
| consensus | ||
| .validate_header(payload.block().sealed_header()) | ||
| .wrap_err("header validation failed")?; | ||
|
|
||
| // Validate incoming header against parent | ||
| consensus | ||
| .validate_header_against_parent(payload.block().sealed_header(), &parent_sealed) | ||
| .wrap_err("header validation against parent failed")?; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
The 3rd issue is that in the payload validation process on default reth, we should pre-validate the incoming payload first prior to execution. This includes validating incoming payload header first which is missing in this p2p builder validation logic - https://github.com/okx/reth/blob/upstream/dev/crates/engine/tree/src/tree/payload_validator.rs#L27
3 participants
📝 Summary
This PR resolve issues found on the payload handler when handling received externally built flashblocks payload. It is found that when retrieving external payloads from peer builders, the state root calculation could mismatch due to errors on the payload validation execution (refer to Issue logs section for the example logs).
evm.transact()instead for execution of transactions and handling of state changes. We should pass the tx value itself directly into evm.transact, allowing the safe conversion into the tx env interface (for revm execution) using upstream conversion methodsIssue logs
✅ I have completed the following steps:
make lintmake test